Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename RescueNode#exception to RescueNode#reference #1204

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

kddnewton
Copy link
Collaborator

Fixes #1160

@kddnewton
Copy link
Collaborator Author

cc @andrykonchin @eregon

The TruffleRuby build is failing because it's referencing the old name for the field. Would it be alright to merge this and have you fix it after?

Going forward, I wonder what we can do to reduce this kind of friction. These kinds of breaking changes of course wouldn't happen outside of a release, but I wouldn't want the TruffleRuby build to be broken for that long.

@eregon
Copy link
Member

eregon commented Aug 4, 2023

@kddnewton The usual strategy for this is to wait that TruffleRuby adopts the change, and then when that's merged we can merge here and no CI breaks.
So if this is not urgent I would wait for that.

@andrykonchin Could you make a small PR to adopt that change?

@kddnewton
Copy link
Collaborator Author

Okay, let me know when it's ready.

@kddnewton
Copy link
Collaborator Author

I'm closing out for the weekend so going to merge this. I really want to figure this out going forward, otherwise it's impossible to make breaking changes without waiting for y'all to make changes. Maybe we could mark the check as optional.

@kddnewton kddnewton merged commit 111adc1 into main Aug 4, 2023
20 of 21 checks passed
@kddnewton kddnewton deleted the rename-to-reference branch August 4, 2023 21:22
@enebo
Copy link
Collaborator

enebo commented Aug 5, 2023

@kddnewton The usual strategy for this is to wait that TruffleRuby adopts the change, and then when that's merged we can merge here and no CI breaks. So if this is not urgent I would wait for that.

@eregon @kddnewton I can see the benefits of wanting people to see breaking changes on TR for bug fixes, but for development work this creates a backwards dependency.

If I submit a PR here (X) to change something then if we followed your process and I was willing to submit a PR (Y) to your repo first (and let's just pretend that is no work but just something which just happens); then I think this assumes YARP development is some single linear process.

So Y is landed, then someone in yarp fixes a different bug with a PR (Z), then we land X. In this case Z is red and has nothing to do with the PR you fixed with Y. That will be confusing to the author of Z. Other scenarios (WIP PRs, language level changes needing githubaction changes depending on language level TR has) but right now this one feels like it would be common.

If we adopted the same process for JRuby, then we have to have two consuming repositories do PRs before the PR can land on YARP. That ignores some of YARPs CI will catch things which would be hard to test locally (so it would be nice to see that before two PRs land in other projects).

I wonder if there is some other way of achieving the goals here. Correct me if I am wrong @eregon but you want to make sure if someone breaks TR from a fix there is a chance for them to notice that and reject the PR (or at least consult about what happened).

@eregon
Copy link
Member

eregon commented Aug 5, 2023

I think the main issue is once it's red nobody will notice if there is another incompatible change or a bug slipping in.
And so it might be hard to get back to green if that happens (e.g. if Java deserialization is broken and has multiple bugs it quickly becomes challenging to fix it, that already happened before the check was there).

I think the existing process of merging in TruffleRuby first and in YARP right after works well for most cases.

The main alternative I see is to avoid breaking that check, for instance in this case by keeping the old field or so. It often feels hacky or overhead though or it might not be reasonably possible.

I also thought at some point to only do the "can it deserialize cleanly" check (maybe running it on JRuby if it doesn't depend on specific YARP node names and field names, or even running Loader on plain java). But that's weaker in terms of testing than actually checking it works for a real usage of YARP, i.e., as TruffleRuby uses it. I think so far this usage is the only one that really checks the AST makes sense and has the relevant information. Nothing else in CI really uses YARP nodes for execution yet, e.g., if there was an incorrect node as the receiver of a CallNode, I think nothing would notice it except that check.

If we do a bunch of field/node renames then it probably makes sense to group them, and similarly for other related breaking changes, so there is less often the need to merge things in a particular order.

st0012 added a commit to Shopify/prism that referenced this pull request Oct 9, 2024
Its `exception` field was renamed to `reference` in ruby#1204 but the
documentation was not updated.
@st0012 st0012 mentioned this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The exception and exceptions fields in RescueNode are confusing a bit
3 participants